Skip to content

Don't recreate adopt instance in every render#12

Merged
pedronauck merged 2 commits intopedronauck:masterfrom
renatorib:patch-2
Apr 27, 2018
Merged

Don't recreate adopt instance in every render#12
pedronauck merged 2 commits intopedronauck:masterfrom
renatorib:patch-2

Conversation

@renatorib
Copy link
Contributor

@renatorib renatorib commented Apr 24, 2018

Fix #11

This change create a adopt() instance at constructor, so it will never change. It's like creating it outside (it really does the same thing).

@renatorib
Copy link
Contributor Author

renatorib commented Apr 24, 2018

I think the failing test changing <Adopt> properties on the fly isn't necessary since it's not needed. You can use mapper fns to change underlying props, and you will never dynamically add some other new component.

Eg.: If you pass Toggle and Value components, you will build your render based on it's props. You can change props passed to Toggle and Value using mapper functions

<Adopt
  passedToggleValue={dynamicValue} // this should works reactively
  mapper={{
    toggle: ({ passedToggleValue }) => <Toggle foo={passedToggleValue} />,
    value: <Value />
  }}
/>
  {({ toggle, value }) => (
     // here my logic uses toggle and value props
  )}
</Adopt>

But you will never dynamically add some third new component (change mapper prop), because your logic will be ever using toggle and value. No more, neither less.

@renatorib
Copy link
Contributor Author

renatorib commented Apr 24, 2018

Also, just a off-topic tip: you can reduce your lib size just refactoring this thing to

- const propsWithoutRest = omit(keys(rest), props)
+ const propsWithoutRest = { children }

You will always know what the props are outside the rest (because you pick it statically).

And this:

- return <Component {...omit(['mapper', 'children'], props)}>{props.children}</Component>
+ const { mapper, ...rest } = props
+ return <Component {...rest} />

You don't need to remove children from the spread operator, and you can remove mapper by excluding it from the rest operator.

So you can remove this omit utility.

@pedronauck
Copy link
Owner

That's a good point @renatorib! I didn't think this way, but really, this make a lot of sense ✌️

@pedronauck pedronauck merged commit 3efdd8a into pedronauck:master Apr 27, 2018
@renatorib renatorib deleted the patch-2 branch May 7, 2018 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments